Skip to content

Conversation

savil
Copy link
Collaborator

@savil savil commented Sep 16, 2024

Summary

For packages without an explicit version, this PR will print the version for
the user in devbox ls.

Fixes #2274

How was it tested?

% devbox ls
* go@latest - 1.23.0
* runx:golangci/golangci-lint@latest - 1.60.2
* runx:mvdan/gofumpt@latest - 0.7.0
* cowsay@3.8.2

Notice that versions are printed for go@latest and the runx packages, but
not for cowsay since it was added via devbox add cowsay@3.8.2

Also, devbox ls -c examples/flakes/php and devbox ls -c examples/flakes/remote

% devbox ls -c examples/flakes/remote
* github:nixos/nixpkgs/5233fd2ba76a3accb5aaa999c00509a11fd0793c#hello
* github:nixos/nixpkgs/5233fd2ba76a3accb5aaa999c00509a11fd0793c#cowsay
* github:F1bonacc1/process-compose/v0.43.1

@savil savil requested review from gcurtis and mikeland73 September 16, 2024 18:30
// Continue to print the package even if we can't resolve the version
// so that the user can see the error for this package, as well as get the
// results for the other packages
resolvedVersion = "<error resolving version>"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe omit the version from the list and print it on stderr at the end?

Comment on lines +835 to +839
if err := p.resolve(); err != nil {
return "", err
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If package is not in lockfile this is just current resolved version, but that's probably ok.

return result
}

func (d *Devbox) AllPackagesIncludingRemovedTriggerPackages() []*devpkg.Package {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change AllPackageNamesIncludingRemovedTriggerPackages to use this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

There's a small subtle difference that I think is okay in practice, but let me call it out.

  1. In AllPackageNamesIncludingRemovedTriggerPackages, the p.VersionedName() will append the Version if it exists
  2. In devpkg's Package.Versioned(), if the version is missing we'll append @latest

I think this doesn't matter in practice, because we always append @latest during devbox add <name>.


// Print the resolved version, unless the user has specified a version already
if strings.HasSuffix(pkg.Versioned(), "latest") && resolvedVersion != "" {
// Runx packages have a "v" prefix (why?). Trim for consistency.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

technically semver has (or can have?) a prefix v

@loreto
Copy link
Contributor

loreto commented Sep 19, 2024

@savil Is this ready to be merged?

@savil savil force-pushed the savil/list-versions branch from 8373b7f to 642058b Compare September 19, 2024 15:52
@savil savil merged commit dd9d373 into main Sep 19, 2024
24 checks passed
@savil savil deleted the savil/list-versions branch September 19, 2024 16:24
Copy link

sentry-io bot commented Sep 24, 2024

Suspect Issues

This pull request was deployed and Sentry observed the following issues:

  • ‼️ **exec.ExitError: <redacted usererr.combined>: nix print-dev-env --json "path:": <redacted exec.... go.jetpack.io/devbox/internal/nix in (*Nix).Pri... View Issue
  • ‼️ *exec.ExitError: nix print-dev-env --json "path:": <redacted exec.ExitError> go.jetpack.io/devbox/internal/nix in (*Nix).Pri... View Issue
  • ‼️ *exec.ExitError: nix print-dev-env --json "path:": <redacted exec.ExitError> go.jetpack.io/devbox/internal/nix in (*Nix).Pri... View Issue

Did you find this useful? React with a 👍 or 👎

mikeland73 added a commit that referenced this pull request Oct 1, 2024
## Summary

Fixes issue introduced in #2277

This comment explains it pretty well:
https://github.com/jetify-com/devbox/pull/2277/files#r1767085866

except:

> I think this doesn't matter in practice, because we always append
@latest during devbox add <name>.

This is not 100% true. In the very small number of packages that are not
in nixhub but are valid attribute paths, (e.g. `stdenv.cc.cc.lib`) we
store the key in the lockfile without `@latest` :(. The key missmatch
caused lockfile.Tidy to remove entries.

My bad for making the suggestion that lead to this, this was really
subtle difference.

## How was it tested?

Added `stdenv.cc.cc.lib` on devbox.json linux only, installed on linux
and copied over lockfile to macos. Did `devbox install` and ensured
lockfile was not modified.
maxcharm093 added a commit to maxcharm093/devbox that referenced this pull request Aug 24, 2025
## Summary

Fixes issue introduced in jetify-com/devbox#2277

This comment explains it pretty well:
https://github.com/jetify-com/devbox/pull/2277/files#r1767085866

except:

> I think this doesn't matter in practice, because we always append
@latest during devbox add <name>.

This is not 100% true. In the very small number of packages that are not
in nixhub but are valid attribute paths, (e.g. `stdenv.cc.cc.lib`) we
store the key in the lockfile without `@latest` :(. The key missmatch
caused lockfile.Tidy to remove entries.

My bad for making the suggestion that lead to this, this was really
subtle difference.

## How was it tested?

Added `stdenv.cc.cc.lib` on devbox.json linux only, installed on linux
and copied over lockfile to macos. Did `devbox install` and ensured
lockfile was not modified.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

The list command should show the installed version
4 participants